Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow users to choose how Commands and EntityCommands should fail #15929

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

JaySpruce
Copy link
Contributor

@JaySpruce JaySpruce commented Oct 15, 2024

Objective

Solution

Added the following Commands:

  • ignore_on_error
  • log_on_error (sends messages using info!)
  • warn_on_error (sends messages using warn!)
  • panic_on_error

and the following EntityCommands:

  • ignore_if_missing
  • log_if_missing
  • warn_if_missing
  • panic_if_missing

These change how subsequent commands will respond if errors occur.

Internally:

  • Added a failure_handling_mode field to Commands.
  • When a fallible Command or an EntityCommand is queued, this field is sent with it.
  • Fallible Commands should check for errors themselves.
  • When any EntityCommand is applied, it automatically checks if the entity exists.
  • If an error occurs, the command matches against failure_handling_mode to determine how to respond.

Showcase

#[derive(Component)]
struct X;

let mut world = World::default();

let mut queue_a = CommandQueue::default();
let mut queue_b = CommandQueue::default();

let mut commands_a = Commands::new(&mut queue_a, &world);
let mut commands_b = Commands::new(&mut queue_b, &world);

let entity = commands_a.spawn_empty().id();

commands_a.entity(entity).despawn();
commands_b.entity(entity).warn_if_missing().insert(X);

queue_a.apply(&mut world);
queue_b.apply(&mut world); // Doesn't panic

Review Notes

The real changes are all in system/commands/mod.rs and world/mod.rs.
All the other files are just returning some commands to their old behavior:

  • despawn() -> warn_if_missing().despawn()
  • remove() -> ignore_if_missing().remove()

These two are the only commands that both 1) didn't already panic by default and 2) were used internally (according to VS Code's "Find All References" anyway).
They might not all need to be kept non-panicking, I just didn't want to regress anything.

Bikeshedding

  • ignore vs silent vs something else?
  • failure_handling_mode vs failure_response vs something else? (Used to be failure_mode, but that's wrong)
  • info! vs debug!?

Possible Additions / Future PRs

  • Deprecate try_ variants of commands
    • I added some comments explaining the change and directing the user to the normal variants, but held off on deprecating since AFAIK that's usually a separate PR.
  • Location tracking
    • Just call Location::caller in the user-facing command (if not set to ignore) and pass it in, so the user can see exactly which command caused an error. Didn't seem to hurt performance meaningfully in my testing, but maybe I'm not the best at benchmarking.
  • Generalized errors for Commands using Result
    • We can't return a Result from a command to the caller, but we could wrap commands in something that handles any Err returned from the internal function call.

Migration Guide

The following commands that used to fail silently will now panic by default:

  • EntityCommands::remove
  • EntityCommands::remove_by_id
  • EntityCommands::remove_with_requires
  • EntityCommands::clear
  • EntityCommands::retain
  • EntityCommands::observe
  • EntityEntryCommands::and_modify

Use ignore_if_missing to achieve the previous behavior:

commands.entity(entity).ignore_if_missing().clear();

The command EntityCommands::despawn used to warn upon failure and will now panic by default.
Use warn_if_missing to achieve the previous behavior:

commands.entity(entity).warn_if_missing().despawn();

try variants of commands are unchanged and will always fail silently; these will be removed in the future.

@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Oct 15, 2024
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help S-Needs-Review Needs reviewer attention (from anyone!) to move forward C-Feature A new feature, making something new possible M-Needs-Release-Note Work that should be called out in the blog due to impact labels Oct 15, 2024
@JaySpruce
Copy link
Contributor Author

JaySpruce commented Oct 16, 2024

I don't know how to do Tracy stuff, but I ran the "spawn_commands" benchmark which does this:

for i in 0..entity_count {
    let mut entity = commands.spawn_empty();
    entity
        .insert_if(A, || black_box(i % 2 == 0))
        .insert_if(B, || black_box(i % 3 == 0))
        .insert_if(C, || black_box(i % 4 == 0));

    if black_box(i % 5 == 0) {
        entity.despawn();
    }
}

and got these results:

main

Code_rKDHOq7DKF

this PR

Code_jD1W6zViIl

@JaySpruce JaySpruce changed the title Allow users to choose how EntityCommands should fail if the entity is missing Allow users to choose how Commands and EntityCommands should fail Oct 16, 2024
@JaySpruce
Copy link
Contributor Author

Maybe controversial, but since invalid EntityCommands just do nothing now, I think it's okay to let Commands::entity return one if the entity doesn't exist. It'll panic if the user wants it to panic, but otherwise it'll just complain or do nothing when they try to use it. Commands::get_entity is still useful if they want to handle it properly.

@JaySpruce JaySpruce marked this pull request as ready for review October 17, 2024 05:30
@JaySpruce
Copy link
Contributor Author

JaySpruce commented Oct 18, 2024

I benchmarked again with a modified version of spawn_commands that despawns some entities early:

main version
for i in 0..entity_count {
    let mut entity = commands.spawn_empty();
    if black_box(i % 3 == 0) {
        entity.despawn();
    }
    entity
        .try_insert_if(A, || black_box(i % 2 == 0))
        .try_insert_if(B, || black_box(i % 3 == 0))
        .try_insert_if(C, || black_box(i % 4 == 0));
    if black_box(i % 5 == 0) {
        entity.despawn();
    }
}
this PR version
for i in 0..entity_count {
    let mut entity = commands.spawn_empty();
    if black_box(i % 3 == 0) {
        entity.despawn();
    }
    entity
        .X_if_missing()
        .insert_if(A, || black_box(i % 2 == 0))
        .insert_if(B, || black_box(i % 3 == 0))
        .insert_if(C, || black_box(i % 4 == 0));
    if black_box(i % 5 == 0) {
        entity.despawn();
    }
}

and got these results:

ignore_if_missing vs main

failure ignore vs main

bad warn_if_missing vs main

failure warn vs main

new warn_if_missing vs main

failure warn vs main better

So, ignore_if_missing is essentially identical in performance to try_ functions in main.

warn_if_missing is about 2x slower, but I think that's to be expected. If it's causing a problem, either fix whatever's despawning entities early or set it to ignore. Scratch that, I fixed it. Apparently Strings are no good.

tl;dr: there's maybe a 1% performance impact across the board, if you don't want to call that noise.

Should note that warn! isn't actually printing anything in these benches (I guess benches and tests just don't have the infrastructure for it), so it'll probably be slower in a real program since printing in general is slow. Still good to know that there's nothing else slowing it down

@JaySpruce
Copy link
Contributor Author

I've got a version working that tracks the calling location of EntityCommands, so that the error messages can be more useful:
Code_aMsZ0VVLHl

It hurts performance of warn_if_missing by another ~2%. Other than that, the biggest downside is that EntityCommands all look like this:

pub fn insert(&mut self, bundle: impl Bundle) -> &mut Self {
    if self.failure_mode().should_respond() {
        self.queue_with_caller(insert(bundle, InsertMode::Replace), Location::caller())
    } else {
        self.queue(insert(bundle, InsertMode::Replace))
    }
}

Could leave it for another PR, or could merge it here if it's wanted

@BenjaminBrienen
Copy link
Contributor

Bikeshedding: I thought a "failure mode" or a "mode of failure" is the way something went wrong. This type/field describes the strategy for handling when something goes wrong.

@JaySpruce
Copy link
Contributor Author

Hadn't heard of that, but that does seem to be the case. Maybe just failure_handling_mode? Doesn't really need to be short since it's not very public-facing.

Totally open to more bikeshedding, I'm not committed to any of the names here

Copy link
Contributor

@BenjaminBrienen BenjaminBrienen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love it!

crates/bevy_ecs/src/system/commands/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/commands/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/commands/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/commands/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/commands/mod.rs Outdated Show resolved Hide resolved
@BenjaminBrienen BenjaminBrienen added D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes and removed S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help labels Oct 28, 2024
github-merge-queue bot pushed a commit that referenced this pull request Dec 6, 2024
## Objective

I was resolving a conflict between #16132 and my PR #15929 and thought
the `clone_entity` commands made more sense in `EntityCommands`.

## Solution

Moved `Commands::clone_entity` to `EntityCommands::clone`, moved
`Commands::clone_entity_with` to `EntityCommands::clone_with`.

## Testing

Ran the two tests that used the old methods.

## Showcase

```
// Create a new entity and keep its EntityCommands.
let mut entity = commands.spawn((ComponentA(10), ComponentB(20)));

// Create a clone of the first entity
let mut entity_clone = entity.clone();
```

The only potential downside is that the method name is now the same as
the one from the `Clone` trait. `EntityCommands` doesn't implement
`Clone` though, so there's no actual conflict.

Maybe I'm biased because this'll work better with my PR, but I think the
UX is nicer regardless.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Needs-Release-Note Work that should be called out in the blog due to impact S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
None yet
3 participants